Skip to content

[FINNA-3708] Refactor LIDO record handling to use VuFindXml. - #199

Open
EreMaijala wants to merge 6 commits into
NatLibFi:devfrom
EreMaijala:dev-lido-xml
Open

[FINNA-3708] Refactor LIDO record handling to use VuFindXml.#199
EreMaijala wants to merge 6 commits into
NatLibFi:devfrom
EreMaijala:dev-lido-xml

Conversation

@EreMaijala

Copy link
Copy Markdown
Contributor

This improves namespace support for LIDO records.

This improves namespace support for LIDO records.
@EreMaijala
EreMaijala requested a review from mshroom June 16, 2026 09:33

@mshroom mshroom left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Muutama kohta tässä, mutta en ehtinyt katsoa vielä kokonaan

Comment thread src/RecordManager/Base/Record/Lido.php Outdated
if (!empty($result)) {
return $result;
}
if ('' !== ($result = $this->xmlDoc->firstValue($place, 'partOfPlace') ?? '')) {

@mshroom mshroom Jun 17, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

partOfPlace ei sisällä tekstiä vaan pitää kutsua tätä getSubLocationia uudestaan niinkuin aiemmin. Tyyliin:

if ($partOfNode = $this->xmlDoc->first($place, 'partOfPlace') {
    $result = $this->getSubLocation($partOfNode, true);
    if ('' !== $result) {
        return $result;
    }
}

Comment thread src/RecordManager/Base/Record/Lido.php Outdated
= $this->xmlDoc->firstValue($placeNode, 'place/namePlaceSet/appellationValue') ?? '';
if ('' !== $appellationValue) {
$mainPlace = $appellationValue;
$placeNode = $this->xmlDoc->first($placeNode, 'place');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

voisiko tässä $placeNode olla vaikka $subPlaceNode, ei toi nyt taida sotkea mitään mutta vaikeuttaa ymmärtämistä kun $placeNode on jo tuossa käytössä eri elementillä

Comment thread src/RecordManager/Base/Record/Lido.php
Comment thread src/RecordManager/Base/Record/Lido.php Outdated
Comment on lines +1336 to +1337
$path = 'lido/descriptiveMetadata/objectIdentificationWrap/repositoryWrap/repositorySet/workID';
return $this->xmlDoc->firstValue(path: $path) ?? '';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nyt tämä palauttaa ekan workID:n vaikka se olisi tyhjä, kun aiemmin käytiin läpi kaikkien repositorySettien ensimmäinen workID

Comment thread src/RecordManager/Base/Record/Lido.php Outdated
Comment on lines +771 to +772
$appellationValueNode = $this->xmlDoc->first($roleNode, 'actor/nameActorSet/appellationValue');
if ($appellationValueNode) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vois yksinkertaistaa tarkistukseen jo tässä että '' !== firstValue($roleNode, 'actor/nameActorSet/appellationValue') koska appellationValueNodesta ei kuitenkaan myöhemmin tsekata mitään muuta

Comment thread tests/fixtures/Base/record/lido-ns.xml Outdated
Comment thread tests/RecordManagerTest/Base/Record/LidoTest.php
@mshroom

mshroom commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Huomiona vaan tähän että namespaceista LIDOssa voi esiintyä lisäksi

Voisi myös vielä sitten testata haravointia myös ilman namespacejen siivoamista, että toimiiko kaikki oletetusti...

@EreMaijala
EreMaijala requested a review from mshroom August 19, 2026 11:06
<<<XML
<lido:lidoWrap $nsPart$schema11>
<lido:lido>
<lido:lidoRecID type="ITEM">123</lido:lidoRecID>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<lido:lidoRecID type="ITEM">123</lido:lidoRecID>
<lido:lidoRecID lido:type="ITEM">123</lido:lidoRecID>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants